Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Audio cues on cell execution completed #165442

Conversation

MonadChains
Copy link
Contributor

Fixes #165085
This PR implement an option in the settings notebook.audioCuesEnabled that enables audio cues on cell execution both in case of success and in case of failure.

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! Looks fine, but it makes me wonder- if you click "Run all" (or run above/below, or select a range and run...) should it sound the audio cue for every cell individually, or just once at the end of the "batch"? I don't know

cc @meganrogge for that question, not sure whether there is a similar concept anywhere else that uses audio cues

src/vs/workbench/contrib/notebook/common/notebookCommon.ts Outdated Show resolved Hide resolved
@MonadChains
Copy link
Contributor Author

Good point! I didn't think about it. I would lean more toward one sound per batch because it can be annoying when you have multiple small cells. On the other hand, having a sound for each cells could provide more informative feedback (I'm thinking of the possibility of counting the cues for knowing which cell is currently being executed). There is also the option of having the user deciding it with an additional setting.

@meganrogge
Copy link
Contributor

@roblourens we sort of encountered this with the build task audio cues and decided to just play one sound at a time instead of capturing the requests and playing them sequentially. #164921

Do the cells get run in parallel? I imagine they don't necessarily finish in order, so it could be pretty confusing for the user if multiple sounds play... for example:

Cell 1
Cell 2
Cell 3

Cell 2 succeeds
Cell 1 fails
Cell 3 succeeds

Could sound misleading given the order the cells appear in is not consistent w the order in which the noise gets played.

@MonadChains
Copy link
Contributor Author

I've changed the behavior of the audio cues. Now the successful execution cue is played on the last cell scheduled, while the failed one always plays whenever a cell fails, since a failed execution interrupts the all the remaining scheduled ones.

@MonadChains MonadChains force-pushed the issue-165085/add-audio-cues-on-cell-execution-completed branch from 9b3ebee to 7ad0f0c Compare November 10, 2022 23:49
@roblourens
Copy link
Member

Every other Audio Cues setting has values like this:

image

please match that pattern. I'm not hearing your audio cue, that's probably because I don't have a screenreader enabled currently, but I hear the others when setting them to On

@MonadChains
Copy link
Contributor Author

MonadChains commented Nov 11, 2022

I'm using the audioCues.taskCompleted cue for successful cells and audioCues.taskFailed for failed ones as proposed in the discussion of the issue. These audio cues must be enabled in the settings along with the newly added option in order to hear the sounds:
Screenshot from 2022-11-11 20-45-07

The checks for the 2 cues are done at the level of the audioCueService:

public async playAudioCue(cue: AudioCue): Promise<void> {
if (this.isEnabled(cue).get()) {
await this.playSound(cue.sound);
}
}

So to bypass it the options are:

  • Modify the audioCueService
  • Create new cues with the same sounds (don't like to duplicate stuff tbh)
  • Call playSound directly in the callback (but this would remove a bit of the abstraction provided by the service)

There is also the option to scrap the new setting completely since cell execution can be considered a "task", and, probably users who turn on the options would likely want to have cues played for notebooks.

What do you think? I'm open to every option.

@roblourens
Copy link
Member

A notebook cell is not a task. Looking at the code, there are more to audio cues than I thought- the notebook AudioCues should be registered with the others here:

so that they should show up in the "List Audio Cues" command like the others. This isn't duplicating stuff- notebook cell execution is its own feature and should be listed and controlled independently.

@MonadChains
Copy link
Contributor Author

I've added the audioCues and removed the old setting. The settings for the cues will appear as audioCues.notebookCellCompleted and audioCues.notebookCellFailed in the settings. I've put the settings in the audioCues group instead of the notebook one because I've seen that the cues for other contribs such as the terminal or the debugger are in the audioCues group too.

roblourens
roblourens previously approved these changes Nov 17, 2022
Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, thanks!

lramos15
lramos15 previously approved these changes Nov 17, 2022
@roblourens roblourens enabled auto-merge (squash) November 17, 2022 15:10
@roblourens roblourens dismissed stale reviews from lramos15 and themself via 35634ff November 17, 2022 16:48
@roblourens roblourens merged commit 5865df3 into microsoft:main Nov 17, 2022
@aeschli aeschli added this to the November 2022 milestone Dec 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for Audio Cues (notebook cell completion)
5 participants